refactor: use primordials in extensions/web #11273

bartlomieju · 2021-07-04T17:04:47Z

Ref #11224

extensions/web/08_text_encoding.js

Co-authored-by: Divy Srivastava <dj.srivastava23@gmail.com>

extensions/web/08_text_encoding.js

Co-authored-by: Divy Srivastava <dj.srivastava23@gmail.com>

SimonRask

08_text_encoding.js - line 389 TypedArrayPrototypeSlice

extensions/web/05_base64.js

SimonRask · 2021-07-04T18:48:56Z

.slice() if you forgot :)

bartlomieju · 2021-07-04T18:51:27Z

.slice() if you forgot :)

Thanks, fixed

lucacasonato

Update Uint8Array.from on line 65 of base64.js

ghost · 2021-07-06T04:35:55Z

extensions/web/08_text_encoding.js

@@ -373,14 +388,16 @@
      if (BOMEncoding === "UTF-8") start = 3;
      else start = 2;
    }
-    return new TextDecoder(encoding).decode(bytes.slice(start));
+    return new TextDecoder(encoding).decode(


should use TextEncoderPrototypeDecode

ditto in text_encoding:

deno/extensions/web/08_text_encoding.js

Line 391 in 8bab7d4

return new TextDecoder(encoding).decode(

TextEncoderPrototypeDecode does not exist. Primordials are only for JS builtins

TextEncoderPrototypeDecode does not exist. Primordials are only for JS builtins

It's prone to prototype manipulation, and that is the purpose of the existance of the primordials in general. Or are they more narrow in their scope/goal?

I see these as things that should be created, because, although I haven't checked the relevant spec for this algorithm, I doubt it specifies performing an ECMAScript Get operation on a TextDecoder instance.

Primordials are limited to JS builtins not external APIs (analogous to its name). Node.js doesn't do TextEncoderPrototypeDecode either

If that is so, then all of my comments here besides the ones about DataView and TypedArray can be discarded/ignored.

Although, why should Deno aim to only do as little as Node.js had?

ghost · 2021-07-06T04:48:20Z

These property accesses should directly use prototype getters: ArrayBufferPrototypeGetByteLength, DataViewPrototypeGetByteLength, TypedArrayPrototypeGetLength. I'm iffy about this line because it accesses a dynamic prototype get accessor, I'd suggest maybe doing something more like

let getByteOffset;
if (value instanceof DataView) {
  length = DataViewPrototypeGetByteLength(value);
  getByteOffset = DataViewPrototypeGetByteOffset;
} else {
  length = TypedArrayPrototypeGetLength(value)
  getByteOffset = TypedArrayPrototypeGetByteOffset;
}

return new (value.constructor)(
  clonedBuffer,
  getByteOffset(value),
  length,
);

also iffy about line 56's buffer access. You'll need an extra branch to get the correct get accessor for the buffer.

This u8 array construction requires an extra branch and usage of primordial accessors.

ghost · 2021-07-06T04:59:44Z

(Unrelated to this PR, but the abort_signal file seem to be missing WebIDL assertions on getters and such, is that handled in configurePrototype?)

Line 107 should use AbortSignalPrototypeGetAborted. I also believe that EventTargetPrototypeDispatchEvent should be used here?

ghost · 2021-07-06T05:13:13Z

Apologies for being as pedantic as I already was, but lastly, the location file needs to use URLPrototypeGet... accessors. :)

littledivy · 2021-07-06T05:41:28Z

These property accesses should directly use prototype getters: ArrayBufferPrototypeGetByteLength, DataViewPrototypeGetByteLength, TypedArrayPrototypeGetLength. I'm iffy about this line because it accesses a dynamic prototype get accessor, I'd suggest maybe doing something more like
let getByteOffset;
if (value instanceof DataView) {
  length = DataViewPrototypeGetByteLength(value);
  getByteOffset = DataViewPrototypeGetByteOffset;
} else {
  length = TypedArrayPrototypeGetLength(value)
  getByteOffset = TypedArrayPrototypeGetByteOffset;
}

return new (value.constructor)(
  clonedBuffer,
  getByteOffset(value),
  length,
);
also iffy about line 56's buffer access. You'll need an extra branch to get the correct get accessor for the buffer.

This u8 array construction requires an extra branch and usage of primordial accessors.

@crimsoncodes0 get accessors of internal primordial objects (eg window.__bootstrap.Dataview) are not exposed to userland; hence they are not vulnerable to prototype pollution

ghost · 2021-07-06T06:00:31Z

These property accesses should directly use prototype getters: ArrayBufferPrototypeGetByteLength, DataViewPrototypeGetByteLength, TypedArrayPrototypeGetLength. I'm iffy about this line because it accesses a dynamic prototype get accessor, I'd suggest maybe doing something more like
let getByteOffset;
if (value instanceof DataView) {
  length = DataViewPrototypeGetByteLength(value);
  getByteOffset = DataViewPrototypeGetByteOffset;
} else {
  length = TypedArrayPrototypeGetLength(value)
  getByteOffset = TypedArrayPrototypeGetByteOffset;
}

return new (value.constructor)(
  clonedBuffer,
  getByteOffset(value),
  length,
);
also iffy about line 56's buffer access. You'll need an extra branch to get the correct get accessor for the buffer.
This u8 array construction requires an extra branch and usage of primordial accessors.
@crimsoncodes0 get accessors of internal primordial objects (eg window.__bootstrap.Dataview) are not exposed to userland; hence they are not vulnerable to prototype pollution

Can you explain what you mean by that, and how it pertains to the code in question?
Was your comment referring to when I'd said

because it accesses a dynamic prototype get accessor

in particular? If not, can you specify which part of my comment you were replying to?

littledivy · 2021-07-06T07:01:53Z

Can you explain what you mean by that, and how it pertains to the code in question?

@crimsoncodes0 The code you are referring to (structuredClone) deals with for example globalThis.DataView and window.__bootstrap.primordials.DataView. So we now have 2 situations:

a. value is passed from userland i.e. globalThis.DataView - Users are allowed to manipulate value's dynamic accessors. (they own the value and how it behaves)

b. value is created internally i.e. window.__bootstrap.primordials.DataView - The getters for value are not prone to prototype pollution at all since window.__bootstrap.primordials.DataView was never exposed to userland.

ghost · 2021-07-06T07:20:10Z

Can you explain what you mean by that, and how it pertains to the code in question?

@crimsoncodes0 The code you are referring to (structuredClone) deals with for example globalThis.DataView and window.__bootstrap.primordials.DataView. So we now have 2 situations:

a. value is passed from userland i.e. globalThis.DataView - Users are allowed to manipulate value's dynamic accessors. (they own the value and how it behaves)

b. value is created internally i.e. window.__bootstrap.primordials.DataView - The getters for value are not prone to prototype pollution at all since window.__bootstrap.primordials.DataView was never exposed to userland.

In situation "a," we obviously need to have the primordials in use to ensure correctness.

As for situation "b"... I don't think I understand the primordial code; how does one have both, a prototype copy, yet also retain correct semantics for comparing the user-provided object against the primordial prototype object via the instanceof operator?

lucacasonato · 2021-07-06T10:24:10Z

@crimsoncodes0 You are correct we will need primordials for these too eventually. These primordials do not yet exist however, so there is nothing to change for this PR.

ghost · 2021-07-06T16:30:25Z

@crimsoncodes0 You are correct we will need primordials for these too eventually. These primordials do not yet exist however, so there is nothing to change for this PR.

From my read of the code, a deep primordial copy is made of both, ArrayBuffer, and TypedArray, so those changes are very possible already, unless I've misunderstood the setup there?

refactor: use primordials in extensions/web

dbde48e

bartlomieju mentioned this pull request Jul 4, 2021

Use primordials everywhere #11224

Closed

67 tasks

bartlomieju requested a review from lucacasonato July 4, 2021 17:09

littledivy reviewed Jul 4, 2021

View reviewed changes

extensions/web/08_text_encoding.js Outdated Show resolved Hide resolved

Update extensions/web/08_text_encoding.js

a7ec3f2

Co-authored-by: Divy Srivastava <dj.srivastava23@gmail.com>

littledivy reviewed Jul 4, 2021

View reviewed changes

extensions/web/08_text_encoding.js Outdated Show resolved Hide resolved

Update extensions/web/08_text_encoding.js

f78dffc

Co-authored-by: Divy Srivastava <dj.srivastava23@gmail.com>

SimonRask suggested changes Jul 4, 2021

View reviewed changes

extensions/web/05_base64.js Outdated Show resolved Hide resolved

fix

2c01923

fix

7a4ffd7

lucacasonato reviewed Jul 5, 2021

View reviewed changes

fix

5fd25e4

bartlomieju requested a review from lucacasonato July 5, 2021 16:27

remove unused

8bab7d4

ghost reviewed Jul 6, 2021

View reviewed changes

try TypedArrayFrom

a37827f

fix again

60c6083

lucacasonato approved these changes Jul 6, 2021

View reviewed changes

bartlomieju merged commit 1aac477 into denoland:main Jul 6, 2021

bartlomieju deleted the primordials_web branch July 6, 2021 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use primordials in extensions/web #11273

refactor: use primordials in extensions/web #11273

bartlomieju commented Jul 4, 2021

SimonRask left a comment

SimonRask commented Jul 4, 2021

bartlomieju commented Jul 4, 2021

lucacasonato left a comment

ghost Jul 6, 2021

ghost Jul 6, 2021

littledivy Jul 6, 2021

ghost Jul 6, 2021 •

edited by ghost

Loading

littledivy Jul 6, 2021

ghost Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021

ghost commented Jul 6, 2021 •

edited by ghost

Loading

littledivy commented Jul 6, 2021

ghost commented Jul 6, 2021 •

edited by ghost

Loading

littledivy commented Jul 6, 2021

ghost commented Jul 6, 2021 •

edited by ghost

Loading

lucacasonato commented Jul 6, 2021

ghost commented Jul 6, 2021 •

edited by ghost

Loading

refactor: use primordials in extensions/web #11273

refactor: use primordials in extensions/web #11273

Conversation

bartlomieju commented Jul 4, 2021

SimonRask left a comment

Choose a reason for hiding this comment

SimonRask commented Jul 4, 2021

bartlomieju commented Jul 4, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

ghost Jul 6, 2021

Choose a reason for hiding this comment

ghost Jul 6, 2021

Choose a reason for hiding this comment

littledivy Jul 6, 2021

Choose a reason for hiding this comment

ghost Jul 6, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

littledivy Jul 6, 2021

Choose a reason for hiding this comment

ghost Jul 6, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Jul 6, 2021 • edited by ghost Loading

ghost commented Jul 6, 2021

ghost commented Jul 6, 2021 • edited by ghost Loading

littledivy commented Jul 6, 2021

ghost commented Jul 6, 2021 • edited by ghost Loading

littledivy commented Jul 6, 2021

ghost commented Jul 6, 2021 • edited by ghost Loading

lucacasonato commented Jul 6, 2021

ghost commented Jul 6, 2021 • edited by ghost Loading

ghost Jul 6, 2021 •

edited by ghost

Loading

ghost Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021 •

edited by ghost

Loading

ghost commented Jul 6, 2021 •

edited by ghost

Loading